Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix instrument edit history vZoom/vScroll issue #2139

Closed

Conversation

alederer
Copy link
Collaborator

@alederer alederer commented Sep 4, 2024

Medium-ish severity (you lose your current undo/redo history when trying to undo past opening a previously unopened macro tab).

Description

Previously, when opening a previously unopened macro tab, all vZoom/vScroll values would automatically get reset from their initial -1 to reasonable values, leading to an undo state being created, which clears redo history. If you would try to undo this, it would revert to -1, leading their values to get reset again, creating an undo step again -- so effectively, opening a previously unopened macro tab cleared your redo history and you could not undo beyond it.

For the fix, my thinking was that vZoom/vScroll should get set up when the instrument is created, rather than remaining -1 until tab opened, so that all these spurious data changes don't happen at all. To get the proper starting values we need data that insEdit previously set up by building a "macroList" of FurnaceGUIMacroDesc in various spots in the code. So the bulk of the refactor is pulling out building the macroList into isolated functions, so that they can be called as part of initializing vZoom/vScroll.

Caveats

Would be very open to ideas for simpler ways to address the issue!

  • Initializing vZoom/vScroll is a FurnaceGUI operation because it currently relies on FurnaceGUIMacroDesc -- a refactor could go further and pull out the parts that are required for setting up vZoom/vScroll? It's weird that vZoom/vScroll are in DivInstrument (an engine struct) to begin with.
  • Is there a bigger macro refactor coming? I saw saw reference to it -- if so this is maybe premature.
  • It's a lot of shuffling code around so I wouldn't be surprised if something subtle broke.

(When testing/messing with with instrument edit undo, please use the history table provided in the Help->Debug window, it shows patch info for the undo/redo queues.)

Adam Lederer added 5 commits September 4, 2024 10:06
Previously, when opening a previously unopened macro tab, all vZoom/vScroll values would get automatically get reset from their initial -1 to reasonable values, leading to an undo state being created, which clears redo history.  If you would try to undo this, it would revert to -1, leading their values to get reset again, creating an undo step again -- so effectively, opening a previously unopened macro tab cleared your redo history and could not be undone-past.

For the fix, my thinking was that vZoom/vScroll should get set up when the instrument is created, rather than remaining -1 until tab opened, so that all these spurious data changes don't happen at all.  To get the proper starting values we need data that insEdit previously set up by building a "macroList" of FurnaceGUIMacroDesc in various spots in the code.  So the bulk of the refactor is pulling out building the macroList into isolated functions, so that they can be called as part of initializing vZoom/vScroll.

(When testing/messing with with instrument edit undo, please use the history table provided in the Help->Debug window, it shows patch info for the undo/redo queues.)
@tildearrow
Copy link
Owner

tildearrow commented Sep 29, 2024

I am not ready to accept this. It changes too many things.

Is there an alternative approach that doesn't require a rewrite of insEdit.cpp?

Moving to an issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants